refactor(block): derive hardfork gating from single-source spec resolution - #362
refactor(block): derive hardfork gating from single-source spec resolution#362RealiCZ wants to merge 38 commits into
Conversation
… floor Pre-block setup — system-contract predeploys, their bytecode versions, and the EIP-2935/EIP-4788 pre-block system calls — was gated on per-fork `is_<fork>_active_at_timestamp` predicates. That reads the hardfork config as a set of independent switches when the domain is a point on a linear ladder, and it makes two shapes behave wrongly. A config that schedules a later fork without its predecessors resolves the executing spec to that fork, yet every predicate below it reports inactive, so the lower forks' setup silently never runs. And a rollback hardfork — `MiniRex1`, live on mainnet, mapping back to EQUIVALENCE — would retract setup that earlier forks already performed if setup were instead gated on the executing spec, dropping the Oracle predeploys and their read-only witness entries from every block in that window. Separate the two questions. `spec_id` stays the reversible executing spec and keeps gating EVM behavior, block limits, and transaction classification. The new `max_activated_spec_id` is the monotone activated-spec floor — the highest spec any activated fork introduced — and gates one-way chain setup. Each contract's `<name>_spec()` builder now takes a resolved `MegaSpecId` rather than a hardfork config, so a per-fork gate cannot be reintroduced, and the executor resolves the floor once per block. `with_all_activated_through(spec)` replaces `with_all_activated().without(f)` as the way to say "a chain running spec N": removing a middle rung leaves later forks active, so both the executing spec and the floor stay at the top of the ladder. The unknown-chain fallback now names its rung instead of inheriting `MegaSpecId::default()`. These chains run their rung from genesis, so it is their semantics from block zero with no fork boundary; introducing a spec must not move them, or history they already produced replays differently through `mega-evme replay`, which resolves unknown chain IDs here.
Six configs still wrote "a chain running Rex5" as `with_all_activated().without(Rex6)` — the shape this change's own guidance now forbids, and the one that silently widens: the next spec leaves the removed fork's successor registered, so the config resolves above the rung its comment claims.
Every external use was `with_all_activated().without(fork)` meaning "a chain running the spec below `fork`", which it does not express: the forks above `fork` stay registered, so the config resolves above the intended rung and climbs again with the next spec. `with_all_activated_through` says it. This narrows an idiom, not a capability — `with(fork, ForkCondition::Never)` still unregisters a fork and must stay public, since the canonical testnet schedule uses it for `MiniRex1` / `MiniRex2`. Writing a gap that way is at least visibly deliberate.
…edicates Spec-introducing per-fork predicates now project the activated-spec floor (cascading, kona-style), so gating on them stays additive on partial ladders; patch forks keep raw event semantics. Hardfork resolution derives from the variant ladder instead of a hand-written chain, the floor scan early-exits at the first activated spec-introducing fork, and validate_schedule() rejects malformed schedules (skipped rungs, ordering, missing params) at load time. The deploy layer no longer sees a hardfork config, and resolve_system_address asserts the floor-above-exec invariant. Costs are pinned by a hardfork_resolution benchmark group.
Claude review status
✅ Review clean Last reviewed: head New this round: 0 finding(s), 0 question(s) · Resolved this round: 0 · Open questions: 0 |
RealiCZ
added
api:breaking
Codecov Report❌ Patch coverage is ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RealiCZ
added
spec:stable
Merging this PR will improve performance by 10.45%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
🧬 Mutation testing — ✅ PASSDiff mutation score: 100.0% (34/34 viable mutants killed)
No new test gaps introduced by this change. 🎉 |
The only skipped-rung test used the lowest rung, whose empty prefix makes the spec-introducing classification degenerate. A middle-rung gap pins the prefix comparison itself, killing the surviving replace-<-with-== mutant.
🧬 Mutation testing — ✅ PASSDiff mutation score: 100.0% (1/1 viable mutants killed)
No new test gaps introduced by this change. 🎉 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f92b95955
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
flyq
left a comment
flyq
left a comment
There was a problem hiding this comment.
Review: derive hardfork gating from single-source spec resolution
Verdict: the design and implementation are strong — I verified the central claims directly on the branch and they hold. Before merge: one coordination-level issue with open PR #360 (inline comment on chain.rs:96) needs an explicit decision, plus two Minors (a nonexistent early-exit implementation claimed by the PR description and a bench comment; a hand-edited mutation test that dropped its mutant linkage).
Verified
- Floor/event parity —
test_floor_matches_per_fork_activation_for_spec_introducing_forkssweeps every canonical schedule's activation timestamps ±1 and pins floor == raw event for every spec-introducing fork; behavior differs only on partial ladders, which no canonical chain has. - Mainnet
MiniRex1rollback window covered end-to-end — the executing spec rolls back toEQUIVALENCEwhile the witness hook still receives the Oracle predeploys' accounts, asserted through a realOnStateHookrecorder rather than inferred. hardfork()rewrite is equivalent —VARIANTS.rev().find(...)preserves the old if-else ladder's order and equal-timestamp tie-break (later-declared wins), and reads raw activation events, correctly bypassing the predicates' semantic change.VARIANTSis macro-generated (alloy-hardforks 0.2.13hardfork!,src/hardfork/macros.rs:15) — no second hand-written ladder to keep in sync.validate_schedule— all four error classes tested, including thewith_all_activated_through(EQUIVALENCE)patch-fork edge and the skipped middle rung (0f92b95); thehardfork_scheduledebug-assert passes on all three canonical schedules.- No leftover callers — no consensus path in
src/orbin/still consumes the projected predicates; the deploy layer is sealed (spec builders take a resolvedMegaSpecId, so a per-fork activation gate cannot be reintroduced). - Partial-ladder e2e tests are high quality — the fail-closed test asserts the specific error string and documents why matching the variant alone would pass under either gating style; exactly the "would this test still pass if the behavior regressed" discipline REVIEW.md asks for.
- Run locally —
mega-evm --lib(276),block_executor(38, incl. the 4 new partial-ladder tests),mutation(47),rex4(216),rex5(209): all green. riscvno_stdcheck passes (ScheduleError'sderive_morederives are no_std-clean).cargo fmtclean; clippy shows only the pre-existing MSRV warning. The new bench group runs; measured numbers match the description's. - Labels are accurate:
api:breaking(signature +withoutprivatization),spec:stable(zero behavior change on canonical schedules, pinned by the parity tests).
Not attachable inline
- [Nit] The PR's Testing section omits the riscv
no_stdcheck (cargo check -p mega-evm --target riscv64imac-unknown-none-elf --no-default-features) — a standard item for core-crate changes. I ran it; it passes. Worth adding to the test plan for the record. - [Nit]
max_activated_spec_idtakesBlockTimestampwhile everyis_*_active_at_timestamppredicate takesu64(the same underlying alias) — worth aligning on one spelling. - The docs updates check out against the implementation: the new "Executing Spec vs. Highest Spec Reached" section in
hardfork-spec.mdmatches the code's semantics, and thereplay.md/ threeAGENTS.mdrule updates all correspond to real code.
Specific change suggestions are attached as inline comments.
🤖 Generated with Claude Code
The exhaustive ladder_index match makes introducing a spec a compile error until the variant is placed, a const assertion ties each ALL entry to its ladder position, and the latest-spec anchor test fails until ALL carries the new spec. Golden name pairs stay hand-written but their spec column must equal ALL.
The descending scan stops at the first activated spec-introducing fork, so floor queries near the top of the ladder cost one or two activation lookups instead of one per fork (~83ns vs ~350ns on the mainnet schedule). A reference test pins the scan against the naive max-over-activated-forks formula across every schedule shape. Spec sweeps in the hardfork tests now come from MegaSpecId::ALL.
Medium-bucket costs (~100M per SSTORE) must still OOG on a REX4 chain: pre-REX5 keeps revm's upstream 30M budget for replay parity, so the budget selection must follow the REX5 spec exactly. Also point the EIP-2935 assertions at the ring-buffer slot the contract actually writes, (number-1) % 8191 — slot 0 was vacuously empty.
A scheduled patch fork now requires its base — the nearest earlier-declared spec-introducing fork — to be scheduled (ScheduleError::OrphanPatch), and with_all_activated_through no longer registers a patch without its base, so through(EQUIVALENCE) registers nothing and resolves by default. The spec-introducing classification and base lookup are factored into MegaHardfork helpers shared by the floor scan, validation, and the builder. All trait timestamp parameters now spell BlockTimestamp.
The executing-spec/highest-spec-reached split and the schedule-climbing rules now use RFC-2119 language, including the orphan-patch prohibition. Mutation probe comments regain their hardfork.rs line references per the REVIEW.md linkage rule.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 568d51c98f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The const assertion's loop guard was invisible to tests: the real ALL always satisfies the property, so a weakened guard passed silently. The check is now a const fn over any list, const-asserted on ALL and fed malformed lists by a test, so the rejection paths exercise the guard.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfc6420ba1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
flyq
left a comment
flyq
left a comment
There was a problem hiding this comment.
Review summary
Verdict: approve with two pre-merge asks. The monotone 1:1 fork→spec ladder with alias specs and the behavior/position dual projection is carried through consistently. What I verified before signing off: is_enabled's truth table is unchanged for every pre-existing spec pair (both sides project through behavior(), identity for non-alias specs); the alias value flowing through CfgEnv.spec is safe in-repo (no raw ordinal comparisons anywhere except two pre-existing, position-correct >= in bin/mega-evme/src/common/state.rs:569-570 — worth migrating to reaches in a follow-up for grep-ability); all three EVM dispatch sites (instruction table, runtime limits, precompiles) route through behavior() with unreachable alias arms; the deploy layer gates on reaches, so the mainnet MiniRex1 window keeps its predeploys and witness entries (pinned end-to-end in tests/block_executor/partial_ladder.rs); validate_schedule covers all four error classes including the elegant alias-without-base collapse into the skipped-rung rule; the REX7 fallback pin has a drift test in both directions; and mutation testing is 100% on the diff.
Two asks before merge, both as inline comments below: the unexplained CodSpeed regression on the two mini_rex subcall benches, and re-verifying the mega-reth serialization claim against current HEAD (I could not verify it independently).
One release-note bullet worth adding: the behavior-fork predicates (is_mini_rex_active_at_timestamp … is_rex_7_active_at_timestamp) changed from raw per-fork event queries to floor projections. On the canonical schedules this is a no-op (pinned by the ±1-sweep parity test) and it is the right gating semantics — but downstream callers using them as "did this fork's activation event occur" on non-canonical configs now get cascaded answers; mega_fork_activation is the raw query. Deserves an explicit line in whatever changelog downstream consumes, beyond the api:breaking label.
Nit on the PR body: resolve_system_address's signature did not actually change (it already took (hardforks, spec, db) on main — only the internal Rex6 gate moved from a timestamp predicate to spec.reaches), so "takes a single spec instead of an exec/setup pair" slightly oversells it.
Merge-order note: landing this before #365 keeps the cheaper rebase on #365's side — the cross-merge conflicts only on executor.rs + instructions.rs, and #365 does not touch spec.rs.
🤖 Generated with Claude Code
RealiCZ
changed the title
|
Thanks — all items handled; the first one was resolved by events between your review base and the current tip. Point by point: CodSpeed (major). Resolved, and the double-projection hypothesis is refuted by the timeline: the two flagged benches went green at Discriminant renumbering (major). Both asks done:
VARIANTS ↔ ladder order (minor). Added Params-drop One correction to the summary: "all three EVM dispatch sites route through Also done: the two |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7bee86616
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Whether this fork introduces a new spec — its spec is strictly higher than every | ||
| /// earlier-declared fork's. Patch forks (`MiniRex1`, `MiniRex2`) do not. | ||
| pub(crate) fn introduces_spec(self) -> bool { | ||
| let declared = self.declaration_index(); | ||
| Self::VARIANTS[..declared].iter().all(|fork| fork.spec_id() < self.spec_id()) | ||
| } |
There was a problem hiding this comment.
Suggest declaring skippability rather than deriving it from the spec map.
introduces_spec() is being used to answer "may a published schedule omit this fork?" — that's what gates the SkippedRung exemption, base_fork(), and the is_*_active_at_timestamp split. But the property it actually needs is why the fork exists: MiniRex1 is omittable because it rolled back a defect only mainnet hit, and MiniRex2 because it undid that rollback. Testnet never hit the defect, so it schedules neither. The spec map doesn't carry that — it selects the right two forks today by coincidence of the current schedule.
Geth treats these as independent axes and declares both. Its skippable forks carry a hand-written optional: true (daoForkBlock, the Glaciers, mergeNetsplitBlock — all situational fixes only some networks needed), and the two axes deliberately don't line up: the Glaciers do change consensus rules and are optional, while Petersburg — which removes an EIP Constantinople added, the closest analogue to MiniRex1 — is not optional and gets a hand-written special case in IsPetersburg. You can't derive one from the other.
Concretely, I'd suggest replacing this with an exhaustive match, so a new fork is a compile error until someone decides — the same forcing function ladder_index already uses:
impl MegaHardfork {
/// Whether a published schedule may omit this fork.
///
/// `MiniRex1` rolled back a `MiniRex` defect on mainnet and `MiniRex2` restored it
/// afterwards; testnet never hit the defect and schedules neither. Every other fork
/// is required — see `validate_schedule`.
pub const fn optional(self) -> bool {
match self {
Self::MiniRex1 | Self::MiniRex2 => true,
Self::MiniRex | Self::Rex | Self::Rex1 | Self::Rex2
| Self::Rex3 | Self::Rex4 | Self::Rex5 | Self::Rex6 => false,
}
}
}Three things fall out, which is what makes me think this is the right cut rather than just a rename:
-
introduces_speccurrently misclassifies a supported shape.AGENTS.mdstates multiple hardforks may map to one spec. The strict<means a futureRex7bsharingREX7withRex7ais silently treated as a patch fork — inheriting the skip exemption and anOrphanPatchdependency it shouldn't have.test_floor_matches_per_fork_activation_for_spec_introducing_forksskips non-introducing forks, so nothing catches it. -
OrphanPatchdisappears. If the no-skip rule is stated in declaration order rather than spec order, "MiniRex1scheduled butMiniRexnot" is already a skipped-fork violation. That also fixes a misdiagnosis in the current rule: a schedule with onlyMiniRex2reportsSkippedRung { missing: MiniRex, scheduled: MiniRex2 }, andMiniRex2is not "scheduled above"MiniRex— it's a patch of the same rung, soOrphanPatchis unreachable for the shape it was added for. The validator then needs nospec_idat all. -
The term "patch fork" can go. It currently collides with the vocabulary twelve lines up in this same file — the enum documents
Rex1–Rex6as "first patch to Rex", "second patch to Rex", which under the new definition are exactly the forks that are not patch forks. It's also load-bearing indocs/spec/hardfork-spec.md:38, where a MUST/MAY rule references "patch hardforks" three times without defining the term anywhere in the document — a third-party implementer can't implement that rule, and guessing from the enum's doc comments gives the wrong set.
with_all_activated_through doesn't need base_fork either — a declaration-order prefix is simpler and is what it actually wants: walk VARIANTS, register until the first fork whose spec isn't enabled, drop the rest.
Not blocking on its own, but I'd want it in this PR rather than a follow-up: once mega-reth calls validate_schedule at genesis load, ScheduleError becomes a cross-repo contract, and reshaping it afterwards is a breaking change that also propagates this vocabulary into a second repo.
There was a problem hiding this comment.
This comment was written against the earlier revision, where forks mapped many-to-one onto specs and introduces_spec() was derived from that map — that code no longer exists. The alias-rung rework replaced it wholesale, so the latest push contains no direct change for this thread; what follows is how the current design answers the same question.
Instead of declaring skippability as an independent per-fork axis (optional()), we made the fork→spec map 1:1 and moved the one hand-written declaration to the spec side: MegaSpecId::behavior(), one arm per alias. "May a schedule omit this fork" is then derived as is_alias() — an alias rung has no setup and no behavior of its own, so omitting it is safe by construction. The two axes geth keeps independent are deliberately collapsed here: an omittable behavior-introducing fork or a mandatory alias is unrepresentable rather than something a validator has to cross-check. MegaETH has neither of geth's diverging cases (no situational forks like the Glaciers, no EIP-level removals like Petersburg — rollbacks here are whole-behavior projections), so the collapse costs nothing today; if a situational behavior fork ever appears, an independent optional() axis comes back exactly as proposed in this comment.
The consequences predicted here did land along the way: OrphanPatch folded into SkippedRung (test_validate_schedule_rejects_alias_without_base), base_fork is gone, "patch fork" gave way to alias with a normative definition in docs/spec/hardfork-spec.md, and the 1:1 strict ascent the derivation rests on is now a compile-time const assertion (climbs_the_spec_ladder).
| /// The behavior this spec executes: alias specs project to the spec whose behavior they | ||
| /// reuse; every other spec is its own behavior. | ||
| pub const fn behavior(self) -> Self { | ||
| match self { | ||
| Self::MINI_REX_1 => Self::EQUIVALENCE, | ||
| Self::MINI_REX_2 => Self::MINI_REX, | ||
| other => other, | ||
| } | ||
| } |
There was a problem hiding this comment.
behavior() is now the most load-bearing table in this design — every is_enabled routes through it twice, is_alias/introduces_behavior derive from it, and the three alias reconciliation tests in instructions.rs, precompiles.rs, and limit.rs all check against it. But nothing pins any property of the table itself: spec.rs has tests for names, ladder positions, order, and is_ladder_prefix, and none for this.
Two shapes are currently expressible and would be caught by nothing:
- A chain — a future
MINI_REX_3 => MINI_REX_1.behavior()returnsMINI_REX_1, an alias, sois_enabledcompares against a rung that has no behavior of its own. Silently wrong, not an error. - A cycle —
A => B, B => A. Single-step lookup, so it terminates and just returns the wrong answer.
Could resolution be made transitive, with cycle detection? i.e. follow the projection to a fixed point, and reject (or fail to compile) if the chain doesn't terminate. That makes MINI_REX_3 => MINI_REX_1 => EQUIVALENCE resolve correctly rather than half-way, and makes a cycle a loud failure instead of a silent wrong answer.
Worth noting the tension so it's a deliberate choice either way: behavior() is const fn and sits on the EVM gate path, so a loop-to-fixed-point turns a table cycle from "wrong answer" into "hang" (at const-eval or at runtime), and detection then needs a bound or a visited set inside no_std + const fn. So transitive resolution and cycle detection have to land together — the first without the second is worse than today.
If you'd rather keep the projection a single lookup, the same two hazards can be made unrepresentable at compile time, following the is_ladder_prefix pattern already in this file:
/// A behavior target must itself be concrete, and must not sit above the spec projecting
/// onto it.
///
/// Idempotence rules out chains *and* cycles in one property: a chain `A→B→C` gives
/// `behavior(behavior(A)) == C != B`; a cycle `A→B→A` gives `A != B`.
const fn behavior_table_is_flat(list: &[MegaSpecId]) -> bool {
let mut i = 0;
while i < list.len() {
let target = list[i].behavior();
if target.behavior() as u8 != target as u8 {
return false; // chain or cycle
}
if target as u8 > list[i] as u8 {
return false; // alias projecting upward
}
i += 1;
}
true
}
const _: () = assert!(
behavior_table_is_flat(MegaSpecId::ALL),
"behavior() targets must be concrete specs at or below the projecting rung"
);The upward check is worth having regardless of which route you take: if an alias projected up, a chain resolved to that rung would execute higher semantics while reaches reported a lower position, so the setup for those semantics would never have run — is_enabled and reaches would be describing incompatible worlds. That's currently true by inspection and by nothing else.
Either way, the checker itself should get a test fed malformed tables — as test_is_ladder_prefix_rejects_malformed_lists already does for the ladder — since the real table always satisfies the property and a weakened guard would pass silently.
| #[test] | ||
| fn test_variants_declaration_order_climbs_the_ladder() { | ||
| // The reverse scan in `hardfork()` and the skipped-rung rule in `validate_schedule` | ||
| // assume declaration order maps to strictly ascending spec rungs; this fails at a | ||
| // misplaced variant instead of two derived tests away. | ||
| for pair in MegaHardfork::VARIANTS.windows(2) { | ||
| assert!( | ||
| (pair[0].spec_id() as u8) < (pair[1].spec_id() as u8), | ||
| "{:?} -> {:?} must climb the spec ladder", | ||
| pair[0], | ||
| pair[1], | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Good that this invariant is pinned at all — but it's guarded more weakly than the ones it underwrites.
The three structural invariants of this design are enforced inconsistently:
| invariant | guard |
|---|---|
MegaSpecId::ALL is in ladder order without gaps |
const _: () = assert!(is_ladder_prefix(...)) — compile time |
every spec has a ladder_index |
exhaustive match — compile time |
| fork → spec is strictly ascending | this #[test] — runtime |
The third is the one the others rest on. max_activated_spec_id collapsing to spec_id is only correct because of it: if a future fork mapped to a lower existing rung, the latest activated fork would no longer be the maximum over activated forks, and one-way setup would start retracting inside that window — the exact bug class this PR exists to prevent, re-enterable through the single invariant the compiler doesn't check.
It's also the one most likely to be broken by a well-intentioned edit. Someone adding a rollback fork the old way — MiniRex3 => MINI_REX, reusing an existing rung rather than minting a new alias rung — trips this test, but only when it runs, and the message ("must climb the spec ladder") doesn't tell them the remedy.
Suggest promoting it to a const assertion, same shape as is_ladder_prefix. It needs only spec_id to become const fn — the body is already a match over unit variants, so it qualifies as-is; the signature would move from &self to self:
const fn variants_climb_the_ladder() -> bool {
let mut i = 1;
while i < MegaHardfork::VARIANTS.len() {
if MegaHardfork::VARIANTS[i - 1].spec_id() as u8 >= MegaHardfork::VARIANTS[i].spec_id() as u8 {
return false;
}
i += 1;
}
true
}
const _: () = assert!(
variants_climb_the_ladder(),
"each hardfork must map to a strictly higher spec rung than the one before it — \
express a rollback as a new alias rung, not by reusing an earlier spec"
);Two things that buys beyond earlier failure: the assertion message can carry the remedy rather than just the violated property, which is what the next person adding a rollback fork actually needs; and strict < pins injectivity at the same time, so the 1:1-ness the docs rely on stops being a separate claim.
Together with the behavior() flatness check I left on spec.rs, that would put all three load-bearing properties on the compiler: the ladder is ordered, forks climb it strictly, and alias projections are flat and point downward.
| /// Returns the highest [`MegaSpecId`] among all [`MegaHardfork`]s activated at or before | ||
| /// `timestamp` — which, under the 1:1 ascending fork->spec map, is exactly | ||
| /// [`spec_id`](Self::spec_id). | ||
| /// | ||
| /// Kept as an alias so call sites can state "one-way setup" intent explicitly; pair it with | ||
| /// [`MegaSpecId::reaches`] (position) rather than `is_enabled` (behavior). | ||
| fn max_activated_spec_id(&self, timestamp: BlockTimestamp) -> MegaSpecId { | ||
| // With the 1:1 ascending fork->spec map, the latest activated fork IS the maximum: | ||
| // the floor coincides with `spec_id` on every schedule. Kept as an alias for the | ||
| // transition; call sites can migrate to `spec_id` + `reaches`. | ||
| self.spec_id(timestamp) | ||
| } |
There was a problem hiding this comment.
Suggest deleting this method — the problem is not that it is redundant, it is that "max" names a concept the model no longer has.
Each hardfork maps to exactly one spec, and the map is strictly ascending in declaration order (now pinned by test_variants_declaration_order_climbs_the_ladder). Under those two facts there is no maximum to take: the set of activated forks has a latest member, and its spec is the answer. A maximum is only a meaningful quantity if the map could descend — which is precisely the shape the 1:1 alias-rung redesign eliminated by giving MiniRex1/MiniRex2 their own ascending rungs.
So the name imports the old mental model. A reader who meets max_activated_spec_id on a public trait will reasonably infer that activation order and spec order can disagree and that the code is defending against it. That inference is now false, and it is exactly the confusion this redesign set out to end. The body being self.spec_id(timestamp) is the symptom; the name is the defect.
Two reasons to do it in this PR rather than later:
- It is new API introduced here, on
MegaHardforks, which downstream implements. Removing it now costs nothing; removing it after merge is a breaking release, and "already published" is how transitional shims become permanent. The doc says "Kept as an alias for the transition; call sites can migrate" — but nothing migrated, so the transition has no end condition. - The stated justification (letting call sites "state one-way setup intent explicitly") is already served by
reaches, at the comparison, which is where the intent actually lives:spec_id(t).reaches(MegaSpecId::REX5) // "the ladder reached REX5" max_activated_spec_id(t).reaches(MegaSpecId::REX5) // states it twice, once via a name that no longer describes a computation
Removal is mechanical — 17 production call sites, all inside mega-evm: the ten predicates below, plus executor.rs:145, deploy.rs:150, oracle.rs:49,111, control.rs:53, limit_control.rs:32, keyless_deploy.rs:51, sequencer_registry.rs:195. The "floor" vocabulary in the surrounding test names and in tests/block_executor/partial_ladder.rs's module doc is the same vestige and can go with it.
One test is worth keeping rather than deleting, under a new name. test_floor_early_exit_matches_naive_reference asserts that the resolved spec equals the maximum over activated forks across canonical, partial, rollback, block-numbered, and empty schedules. The early-exit scan it was written for is gone, but what it now checks is the ascending invariant itself, verified dynamically across schedule shapes — a good complement to the static check, since one proves declaration order climbs and the other proves the resolved spec really is the maximum on every shape a schedule can take. Something like test_resolved_spec_is_the_maximum_over_activated_forks would say what it now does.
| /// The scheduled spec for this block's timestamp, resolved once at construction. | ||
| /// | ||
| /// Every pre-block setup gate derives from this single value through `reaches` (position), | ||
| /// which keeps setup additive by construction and immune to alias windows — an alias rung | ||
| /// rolls back behavior, not the setup below it. | ||
| /// | ||
| /// Cached because the block env is fixed for an executor's lifetime — the constructor | ||
| /// already reads `block().timestamp` for its hardfork-coherence asserts. | ||
| setup_spec: MegaSpecId, |
There was a problem hiding this comment.
Suggest dropping this field and resolving from one source instead.
Its value is hardforks.spec_id(block_timestamp), and it is read only inside pre_execution_changes and its one helper (lines ~196-346). Nothing later in the executor's lifetime touches it, so the caching rationale is thin — the value is resolved once per block either way.
The name is the same residue as max_activated_spec_id, one layer up. The field doc now correctly locates the setup-vs-execution distinction in reaches ("through reaches (position)"), which is the argument against also encoding it in the value's name: if the comparison carries the distinction, the binding does not need to. scheduled_spec would at least match the doc's own words, but inlining is cleaner.
The substantive part is that there are now two sources of the same fact inside one pre-execution chain:
let setup_spec = self.setup_spec; // :196 — hardforks.spec_id(ts)
...
let spec = self.evm.ctx().mega_spec(); // :695 — cfg_env.spec
resolve_system_address(&self.hardforks, spec, ...) // :697Both answer "what spec is this block", and they are reconciled only by the coherence assert in new():
#[cfg(not(any(test, feature = "test-utils")))]
assert_eq!(evm.spec_id(), hardforks.spec_id(block_timestamp), ...);which is compiled out under test and test-utils. bin/mega-evme/Cargo.toml enables test-utils unconditionally, so the two can diverge there: under replay --override.spec rex5 on a pre-Rex2 block, pre-block setup follows the schedule and installs the v1.0.0 Oracle while resolve_system_address follows the override — REX5 semantics executing against pre-REX2 predeploys.
To be clear about scope: that divergence is pre-existing, not introduced here (setup resolved from hardforks before this PR too), so I would not block on it. It is worth surfacing because this PR is what made "which spec gates what" explicit, and because it points at the fix for both issues at once: use one source throughout. resolve_system_address already takes self.evm.ctx().mega_spec(); having pre_execution_changes read the same value would make the executor coherent by construction rather than by an assert that is absent from precisely the builds where divergence is reachable.
Worth deciding deliberately either way — if the schedule (not the cfg) is intended to be authoritative for setup even under an explicit spec override, that is a defensible choice, but it should be stated in the field doc rather than left as a consequence of which of the two bindings a given call site happened to reach for.
Pre-block setup now follows the executing spec, so the rex4 rows previously running Rex5-scheduled setup measure a REX4 world from here on; the affected CodSpeed baselines shift accordingly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 574bd50137
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
vincent-k2026
left a comment
vincent-k2026
left a comment
There was a problem hiding this comment.
Theme: the alias-rung model turns "the fork→spec map must not descend" from a maintained convention into something structurally unrepresentable, and — unusually — the guards that enforce it are themselves tested. Approving; the items below are non-blocking.
Verified against HEAD:
cargo test -p mega-evm → 1253 passed / 0 failed
Then two deliberate mutations, to check the guards actually discriminate rather than merely exist:
| mutation | result |
|---|---|
behavior() → MINI_REX_2 => MINI_REX_1 (an alias chain) |
compile error: error[E0080]: behavior() targets must be concrete specs at or below the projecting rung |
instructions.rs regroups MINI_REX_1 under the mini_rex table |
test red: MINI_REX_1 table diverges from its behavior target EQUIVALENCE at opcode 0x00 |
Both fail as designed.
Prior rounds
Four of the five open threads are addressed at HEAD and can be resolved: behavior() flatness is now pinned by the is_flat_projection const assertion (verified firing above); the ladder-climb check is a const assertion (climbs_the_spec_ladder); max_activated_spec_id is gone (zero references repo-wide); and the executor's setup_spec field is gone in favour of self.evm.ctx().mega_spec(). The fifth (optional() as an independent axis) has RealiCZ's design answer and was superseded wholesale by the alias-rung rework.
Genuinely good
- The model is the fix. Making fork→spec 1:1 and monotone, and expressing rollbacks as alias rungs, deletes the machinery a reversible map needed —
max_activated_spec_iddisappears because there is no longer a maximum to take. That is a structural elimination, not a refactor. - The checkers are tested.
is_ladder_prefix,is_flat_projection, andclimbs_the_spec_ladderall take&[T]so the real table goes through a const assertion while malformed tables go through unit tests. Most codebases test the guarded thing and not the guard, which is exactly how a weakened guard ships silently. is_flat_projectioncovers chains, cycles, upward targets, and duplicate keys as one fixed-point property rather than four separate checks.- "State it twice with an automatic cross-check" at every dispatch site: the hand-written alias grouping in
instructions.rs/precompiles.rs/evm/limit.rs/limit.rsis reconciled againstbehavior()by a generic test driven offALL × is_alias(). Instruction tables compared per opcode withfn_addr_eq, precompile sets by pointer identity — the comparison forms are well chosen. resolve_system_addressreads one spec through two projections with the reason stated at the call site:is_enabledgates whether dynamic resolution applies,reachesselects the installed bytecode version.test_missing_params_fail_at_load_time_and_pre_block_alikeis genuinely two-sided — it asserts thevalidate_scheduleerror variant and drives a realpre_execution_changes()failure, rather than restating one side.withoutmade private with a doc that names the idiom it removes, not just the API:with_all_activated().without(fork)reads as "the spec belowfork" and is not.- The unknown-chain fallback is pinned to a named rung, and its test comment is honest that a new spec leaves it green — it pins drift, not the decision.
Non-blocking
1. validate_schedule never runs in a release build. Its only production call site is the debug_assert_eq! in chain.rs:116, which is compiled out. The trait doc, block/AGENTS.md's invariant map, and the README all say node startup should call it, and nothing does — here or in mega-reth. So the fail-fast half of this design ships inert: a published schedule with a skipped rung or missing params still reaches block execution and leans on the fail-safe (position-compared setup stays additive). mega-evme and mega-t8n both resolve chain configs and could be the first real caller; failing that, please link a tracked mega-reth follow-up in the PR body rather than leaving a "should".
2. The required-params roster is the one hand-maintained parallel list left. validate_schedule hardcodes Rex5 → SequencerRegistryConfig / Rex6 → SequencerRegistryRex6Config; executor.rs and transact_deploy_sequencer_registry_for hardcode the same two rules independently; the pairing test hardcodes them a third time. A third HardforkParams type added without touching all three goes unnoticed — the pairing test does not grow on its own. The doc concedes this ("A new HardforkParams type must be registered here"). A single const table — &[(MegaHardfork, &'static str, fn(&dyn MegaHardforks) -> bool)] — iterated by both validate_schedule and the pairing test would put this on the same footing as everything else in the PR.
3. Nothing forbids an alias rung that crosses a one-way setup boundary. is_flat_projection only requires the target be downward and a fixed point. A future alias at, say, rung 12 projecting to EQUIVALENCE satisfies both, and yields a hybrid: reaches(REX5) installs the Rex5+ Oracle v2.0.0 and the SequencerRegistry, while resolve_system_address's is_enabled(REX5) gate (REX5.behavior()=9 <= EQUIVALENCE.behavior()=0 → false) returns MEGA_SYSTEM_ADDRESS. The executor then uses the static address while the on-chain Oracle reads the registry — the hybrid-spec class this PR exists to prevent, re-enterable through the one axis nothing checks. Unrepresentable today (both aliases sit below REX), so not blocking. Worth either a normative rule in docs/spec/hardfork-spec.md ("an alias rung must not cross a one-way setup boundary; the lowest is MINI_REX") or a named test that enumerates the reaches-gated rungs and asserts every alias sits below the lowest one.
4. The replay-safety audit covers mega-reth but not mega-kona. mega-kona's crates/proof/megaevm/src/lib.rs also implements MegaHardforks, over a MegaChainSpec with one Option<u64> per fork — precisely the partial-ladder shape where the is_*_active_at_timestamp change (raw event → cascading position projection) is observable — and it never calls validate_schedule. It is pinned at tag v1.7.0, so nothing breaks today; but its mega_fork_activation is an exhaustive match over MegaHardfork with no Rex7 arm, so the next bump touches that code anyway, and that is the moment to re-derive its predicates. This is a fault-proof component, so a predicate semantics change is consensus-relevant — worth naming it in the release-note section.
5. State the discriminant-renumbering audit as a command, not a conclusion. MegaSpecId is #[repr(u8)] with derived Serialize/Deserialize, and this PR moves REX..REX7 from 2..9 to 4..11 — an old REX(2) decodes as MINI_REX_1(2), i.e. EQUIVALENCE semantics, under any variant-index codec. test_ladder_positions_are_pinned's own comment names this hazard. I checked what I could reach: EvmEnv<MegaSpecId> is never serialized in mega-reth or stateless-validator, and stateless-validator's bincode witness format is (SaltWitness, MptWitness) with no spec field — so the claim holds there. But gh search code MegaSpecId --owner megaeth-labs does not even return stateless-validator's four local references, so it under-reports and cannot support a "zero references" conclusion. Either list the repos and the command used, or make MegaSpecId's serde name-based (JSON behaviour unchanged, compact codecs immune to renumbering from here on).
Nits
is_enabled(alias)is unconditionally true and perfectly legal.spec.is_enabled(MINI_REX_1)projects the argument toEQUIVALENCE, so any behavior gate written against an alias rung is a silent no-op.reaches(alias)is meaningful;is_enabled(alias)never is. Worth a line in theis_enableddoc (which currently only says both sides project), or a test that pins the property so it reads as deliberate.- mega-reth's
revm_spec()is a second statement ofspec_id:chain_spec.hardfork(ts).map(|h| h.spec_id()).unwrap_or(EQUIVALENCE), with its own fallback. Now thatMegaHardforks::spec_idis trait API, that call site should just use it so the fallback cannot diverge — a line in the PR description would be enough to route it. BlockLimits::from_hardfork_and_block_gas_limitstill keys onMegaHardfork, the only dispatch site where a fork selects behavior; the other three moved toMegaSpecId. Making itfrom_specwould let the same generic reconciliation cover it.- The Performance section contradicts itself: "zero instruction-count change across all pre-existing benchmarks" versus "the
rex4rows ... their CodSpeed baselines shift accordingly". The second is the accurate one; the first needs narrowing.
4 participants
Summary
MINI_REX_1(behavior:EQUIVALENCE) andMINI_REX_2(behavior:MINI_REX) — rungs of their own whosebehavior()projects to an earlier spec. The resolvedspec_idonly ever climbs; mainnet's rollback window resolves toMINI_REX_1and executesEQUIVALENCEsemantics through the projection.spec.is_enabled(X)compares behavior (both sides project, so alias windows roll execution semantics back — the ~30 existing behavior gates inlimit/,intercept, etc. needed zero changes), andspec.reaches(X)compares position (one-way chain setup: predeploys, bytecode versions, pre-block EIP-2935/4788 rules — never rolled back). The EVM dispatch sites (instruction tables, runtime limits, precompiles, block limits) give each alias a first-class arm grouped with its target (EQUIVALENCE | MINI_REX_1 => ...), and generic reconciliation tests (driven offMegaSpecId::ALL×is_alias()) assert each site's grouping agrees withbehavior()— stating the mapping twice with an automatic cross-check, so a mis-grouped arm fails CI instead of shipping a hybrid spec.MegaSpecId::ALLis a gapless ladder prefix, thebehavior()projection is flat (no alias chains or cycles, no upward or duplicate-keyed targets), and the fork→spec map climbs the ladder strictly (strictness doubles as the 1:1 proof, and the assertion message carries the remedy: express a rollback as a new alias rung, never by reusing an earlier spec).crates/mega-evm/src/block/AGENTS.mdgains an invariant map indexing every guard by the layer it fires at (compile / config-load / block-execution / test / CI).max_activated_spec_idis gone — the maximum over activated forks IS the latest activated fork's spec, sospec_idis the one resolution and one-way intent lives at the comparison (reaches). Pre-block setup reads the executing spec from the EVM cfg — the same sourceresolve_system_addressreads — so the executor is coherent by construction rather than by an assert, and the orphan-patch validation rule folds into the general skipped-rung check (an alias scheduled without its base is a skipped rung, since the alias's rung sits above its base's).is_<fork>_active_at_timestamppredicates are position projections for behavior-introducing forks (kona-style cascading: a schedule that omits a predecessor still reports it active, keeping downstream gates additive) and raw event queries for the two alias forks, whose occurrence is not recoverable from ladder position — testnet never scheduled them while its ladder climbs past their rungs.MegaHardforks::validate_schedule(op-node-styleCheck()): a published schedule must climb the ladder without skipped rungs (alias forks may be omitted, but an alias without its base is itself a skipped rung), in activation order, by timestamp, with required per-fork params attached (Rex5→SequencerRegistryConfig,Rex6→SequencerRegistryRex6Config);hardfork_scheduledebug-asserts it and node startup can call it to fail fast while block execution stays tolerant of malformed schedules. A pairing test pins that the load-time params roster and the pre-block fail-closed checks cover the same requirements on the same config.hardfork()resolution fromMegaHardfork::VARIANTSin reverse declaration order and declareMegaSpecId::ALLas the single spec enumeration with an exhaustive-match compile guard — porting REX7 (merged from feat: seal REX6 and open REX7 #360) exercised exactly this forcing: the merge would not compile until the new spec was placed on the ladder. Hand-written spec rosters derive fromALLwhere possible (--bench-specaccepted-value list and its test).with_all_activated_through(MegaSpecId::N)(withoutis now private), and pin the unknown-chain fallback to a named rung — advanced toREX7in this PR to match the fallback behavior feat: seal REX6 and open REX7 #360 shipped, as an explicit constant rather than an inherited default.<name>_spec()builder andtransact_deploy_sequencer_registry_fortake the resolved spec (plus typed params) instead of a hardfork config, so a per-fork activation gate cannot be reintroduced.Replay safety
behavior()projection shared by every gate — no duplicated instruction tables, no second frozen surface. The mainnetMiniRex1window is covered end-to-end: a full block executes underCfgEnv.spec = MINI_REX_1, producesEQUIVALENCEsemantics, and keeps emitting the MiniRex predeploys' read-only witness entries.MegaSpecIdvariants (no exhaustive matches, no numeric ordinal use, no numeric serialization) — the ladder renumbering and the alias value flowing throughCfgEnv.specare transparent to it; predicates keep their semantics through the trait defaults.Performance
hardfork_resolutiongroup pins absolute costs.hardforks_for(spec)), so a row measures one complete per-spec world; therex4rows previously ran Rex5-scheduled pre-block setup and their CodSpeed baselines shift accordingly.API changes
MegaHardforkConfig::withoutis no longer public (usewith_all_activated_through, orwith(fork, ForkCondition::Never)for a deliberate gap).is_<fork>_active_at_timestamppredicates for behavior-introducing forks are now position projections rather than raw per-fork event queries — identical on the canonical schedules (pinned by ±1-sweep parity tests), but on non-canonical configs a scheduled fork now implies its predecessors' answers;mega_fork_activationremains the raw event query.mega-evme replay --override.specnow applies the override to pre-block setup as well — setup and execution move to the override spec together (a coherent what-if, matchingrun/tx) instead of setup following the chain schedule while execution follows the override; replay without an override is unchanged. Block-level limits still derive from the chain schedule under an override; aligning them is queued as a mega-evme follow-up.MegaSpecIdgainsMINI_REX_1/MINI_REX_2(andREX7via feat: seal REX6 and open REX7 #360);is_enabledcompares behavior projections (unchanged truth table for all non-alias pairs);MegaHardfork::spec_id()is 1:1 andconst."MiniRex1"/"MiniRex2"become valid spec names inFromStr/--override.spec.MegaSpecId::{ALL, behavior, is_alias, reaches},MegaHardforks::validate_schedule,ScheduleError,MegaHardforkConfig::with_all_activated_through.Testing
with_all_activated_throughresolution per rung (driven offMegaSpecId::ALL, including alias rungs), the resolved spec equalling the naive maximum over activated forks on every schedule shape, allvalidate_scheduleerror classes, alias reconciliation at every dispatch site (instruction tables, precompiles, runtime limits, block limits), malformed-input rejection for all three const checkers, and the latest-spec anchor.no_stdcheck pass.